Conversation
ghost
self-requested a review
as a 
|
While we're on the subject, am I the only one who thinks the word "useless" sounds rude? I liked the previously suggested "unnecessary defensive programming". Do I understand it correctly that this will not introduce any new alerts, but merely reclassify existing ones into this less severe query? It's not quite clear from bullet 3 if this means new alerts. |
|
The phrasing is inherited from Brand new alerts will be added because of some additional constant folding steps in this query. Consider the simple case of: if (u !== undefined) {
u.p;
}This is obviously not flagged by typeof f === "function" && f();Maybe we should let |
|
Ping @Semmle/doc on whether "useless" is considered rude; as a non-native speaker I don't feel qualified to judge. |
xiemaisi
left a comment
xiemaisi
left a comment
There was a problem hiding this comment.
Wow, that's a lot of new code! A few comments and suggestions.
Overall, the large number of syntactic checks seems quite cumbersome and I'm not confident I fully understand all of the patterns being checked for. This is the sort of thing we managed to avoid in the type inference by using a more precise lattice for analysing refinement conditions, but such an approach may well not apply here. If it helps us avoid false positives that's good.
Agreed, but I believe we must use syntactic checks to identify the relevant programming patterns. This could be simplified a bit if we added constant folding for boolean results to the type inference, but we decided that it may not be worth it for just this query and
I have added source code examples to the docstrings. |
|
Rebased on current master to make the tests for |
semmledocs-ac
left a comment
semmledocs-ac
left a comment
There was a problem hiding this comment.
I've commented on the use of "useless" but haven't done a review as such.
|
Asked @semmledocs-ac for help as I'm not a native speaker either. Please let me know when this is ready for a doc review. Thanks. |
|
@mc-semmle I have addressed the "is useless" comments, and this PR is ready for doc review. |
| @@ -0,0 +1,32 @@ | |||
| /** | |||
| * @name Useless defensive code | |||
There was a problem hiding this comment.
Are we happy with the use of the word "Useless" in the query name?
| @@ -0,0 +1,32 @@ | |||
| /** | |||
| * @name Useless defensive code | |||
| * @description If the situation some defensive code guards against never | |||
There was a problem hiding this comment.
Sorry but I don't understand what you mean in the description. Would it be possible to rephrase so it's clearer?
|
|
||
| However, if the situation that some defensive code guards | ||
| against never can occur, then the defensive code serves no purpose and | ||
| it can safely be removed. |
There was a problem hiding this comment.
Remove "it"?
...then the defensive code serves no purpose and can safely be removed.
| <p> | ||
|
|
||
| However, due to coercion rules, <code>v == | ||
| undefined</code> holds for both the situation where <code>v</code> |
There was a problem hiding this comment.
Same here - remove "it"?
so the v == null guard serves no purpose, and can be removed:
|
@esben-semmle - doc review completed. This LGTM, I have just made a few minor comments / suggestions. Feel free to ignore them if you don't agree. |
|
Thank you @mc-semmle The query description is now "Defensive code that guards against a situation that never happens is not needed." And the query name is "Unneeded defensive code" |
|
Thank you for the updates @esben-semmle - LGTM 👍 |
mchammer01
left a comment
mchammer01
left a comment
There was a problem hiding this comment.
Good to go from my perspective. Thanks for the last amendments.
ghost
mentioned this pull request
|
Conflicts. |
|
Conflicts resolved. |
4 participants
This PR introduces
js/useless-defensive-codeas arecommendationquery. The query is based on syntactic patterns, and type inference for a variable in each pattern.Defensive code is typically a condition that prevents
nullorundefinedvalues from causing a crash later on. Defensive code is useless when the condition evaluates to a single value for all execution paths. In that case, the condition might as well be removed. I welcome a discussion of the patterns.The end result is that:
js/trivial-conditionalandjs/comparison-between-incompatible-typesare whitelistedjs/useless-defensive-codejs/useless-defensive-codeThe most important consequence of this is that the remaining alerts from
js/trivial-conditionalandjs/comparison-between-incompatible-typesnow are much more severe on average. A secondary consequence is that alerts caused by type inference improvements will be easier to judge the value of.Please note that the query deliberately treats cases like
o === undefined || o.pando === undefined && o.pidentically, for the sake of simplicity. It is the responsibility of another query to flag the definite error in the latter case.Bullet 1 and 2 above may cause a special kind of wobbliness: if we gain precision because of a simpler program or because of an improved analysis implementation, then additional alerts may be flagged by
js/useless-defensive-code, while a more complex program may change those alerts tojs/trivial-conditional. C.f. another evaluation.An oldish evaluation shows that performance is fine and an interesting amount of result changes, with a net additional 69 alerts that the programmer will be presented for by default. A smaller, but more recent, evaluation on top of the current master shows that this still scales.